fix: gate xblock action menu on authz edit/manage_tags permissions - #39050
jacobo-dominguez-wgu wants to merge 2 commits into
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @jacobo-dominguez-wgu! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
mphilbrick211
added
the
mao-onboarding
46a3067 to
5ed8656
Compare
carlos-marquez-wgu
left a comment
carlos-marquez-wgu
left a comment
There was a problem hiding this comment.
Tested and working locally
BryanttV
left a comment
BryanttV
left a comment
There was a problem hiding this comment.
Thanks @jacobo-dominguez-wgu!
| can_edit = has_studio_write_access(request.user, usage_key.course_key) or ( | ||
| is_authz_authoring_enabled and authz_can_edit_course_content | ||
| ) | ||
|
|
There was a problem hiding this comment.
The problem with this or is that legacy permissions are always being validated even if AuthZ is enabled, and we don't want that.
And now that I'm checking the code again, one suggestion to simplify this further: is_authz_authoring_enabled doesn't need to reach the template at all.
can_edit and can_manage_tags could each resolve their own "flag is off" default internally (e.g. can_edit delegating entirely to user_has_course_permission(..., legacy_permission=WRITE), and the tags check returning True when the flag is off, since tag management has no legacy-permission concept). That way only two already-final booleans (can_edit, can_manage_tags) need to reach studio_xblock_wrapper.html.
This also fixes another bug: "Manage Tags" is nested inside the div gated on edit_course_content, so a role granted only manage_tags (no edit access) could never reach it. The div should open on can_edit or can_manage_tags instead.
What do you think?
There was a problem hiding this comment.
Thanks @BryanttV! Refactored so only the two already-final booleans reach the template.
Two follow-ups worth flagging that came out of this:
-
Once the div opens for a manage-tags-only user, the edit-type items inside (Move, Manage Access, Copy, Duplicate, Delete) were gated on flags that didn't account for can_edit, so they'd leak. Gated those on can_edit at the source (can_add/can_move now and can_edit; Copy on is_course and can_edit), matching how can_edit_visibility already worked.
-
The div gate also references can_edit_title, which was hardcoded True, so the div always rendered. Made can_edit_title permission-aware too: True when the flag is off, else courses.edit_course_content.
46a3067 to
fd9f9bc
Compare
fd9f9bc to
9238420
Compare
BryanttV
left a comment
BryanttV
left a comment
There was a problem hiding this comment.
Thanks for making these changes, the code looks much better! LGTM. Could we update the cover letter to reflect what was done?
jacobo-dominguez-wgu
changed the title
Done, thanks! |
mariajgrimaldi
left a comment
mariajgrimaldi
left a comment
There was a problem hiding this comment.
Just a few comments :)
Thanks!
| if not enable_authz_course_authoring(course_key): | ||
| return True |
There was a problem hiding this comment.
Can't we move this condition (legacy permission = None AND flag off) to user_has_course_permission? So we don't have to check this twice when the flag is on?
There was a problem hiding this comment.
I have moved it to user_has_course_permission by adding a default_fallback prop, also added new test to cover it, thanks!
0f6a2af to
46a3067
Compare
Centralize the "flag off + no legacy permission" default in user_has_course_permission so _user_can_manage_tags and _user_can_edit_title don't repeat the flag check. Add tests for the new parameter and switch two test asserts to plain asserts.
3213669 to
399dc61
Compare
This branch has not been deployed
6 participants
Description
This change fixes how the Studio XBlock component card ("header actions" menu) decides which authoring affordances to show when the AuthZ course-authoring rollout (
authz.enable_course_authoring) is enabled, and simplifies how those decisions reach the template.Problems fixed
Legacy access was validated even when AuthZ was enabled.
can_editwas derived fromhas_studio_write_access(...)OR'd with the AuthZ result, so legacy studio write access was always honored — even on a course where AuthZ is enabled and denies the user.user_has_course_permission(...)already encapsulates the correct logic (AuthZ-only when the flag is on, legacy fallback when off), so the extra legacy check was both redundant and wrong."Manage Tags" was unreachable for a tags-only role. The gating flags were plumbed through the template as separate
is_authz_authoring_enabled/authz_can_edit_course_content/authz_can_manage_tagsvalues, and the "Manage Tags" item was nested under a condition tied to edit access. A role granted only courses.manage_tags(no edit) could not reach it. The div now opens oncan_edit or can_manage_tags or can_edit_title`.Edit-type actions would leak to a tags-only user. Opening the menu for a tags-only user exposes the edit-type items inside (Move, Manage Access, Copy to Clipboard, Duplicate, Delete), which were gated on flags that did not account for
can_edit.can_edit_titlewas hardcodedTrue. The div-open condition referencescan_edit_title, which was alwaysTrue, so the div rendered regardless of permissions.Supporting information
Fixes openedx/openedx-authz#418
Testing instructions
Automated:
Run the regression tests added in
TestXBlockViewHandlerHeaderActionsAuthz:They cover the component edit button on a leaf
htmlcomponent preview:edit_course_contentgranted + no legacy write access → edit button shownManual:
Prerequisite: a course with
authz.enable_course_authoringenabled and roles that grant granular permissions (courses.edit_course_content,courses.manage_tags).edit_course_contentand nomanage_tags): open a unit in Studio. The component card action menu shows Edit, Manage Access, Move, Copy to Clipboard, Duplicate, Delete, and no Manage Tags.header-actionsmenu does not render at all.manage_tags, andedit_course_content): open a unit. The action menu is reachable and shows Manage Tags.Demo
User with edit_course_content permission must be able to edit course content.
Screen.Recording.2026-08-31.at.3.16.16.PM.mov
AI usage notice
Used Copilot with auto model mode (Claude) to assist on the modification and creation of unit tests.
Important
Needs to be reviewed after #39013, it contains code from that pr.